src/table/mod.rs (2)

570-577: Consider validating the block type for range tombstones.

The recovery path loads the range tombstone block with CompressionType::None but doesn't validate that the loaded block has BlockType::RangeTombstone. This differs from other block loading paths (e.g., filter block at lines 553-560) that verify the block type.

♻️ Proposed fix

         let range_tombstones = if let Some(rt_handle) = regions.range_tombstones {
             log::trace!("Loading range tombstone block, with rt_ptr={rt_handle:?}");
             let block = Block::from_file(&file, rt_handle, crate::CompressionType::None)?;
+            if block.header.block_type != BlockType::RangeTombstone {
+                return Err(crate::Error::InvalidTag((
+                    "BlockType",
+                    block.header.block_type.into(),
+                )));
+            }
             Self::decode_range_tombstones(block)?
         } else {
             Vec::new()
         };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/table/mod.rs` around lines 570 - 577, The range tombstone loading path
doesn't validate the loaded block type; when regions.range_tombstones is Some
you call Block::from_file(..., crate::CompressionType::None) and then
Self::decode_range_tombstones(block) without checking block.block_type. Update
the code to verify the loaded block's type equals BlockType::RangeTombstone (or
return an error if not) before calling Self::decode_range_tombstones; reference
the regions.range_tombstones branch, the Block::from_file call,
crate::CompressionType::None, and Self::decode_range_tombstones to locate and
modify the logic.

---

620-660: Consider validating tombstone invariant start < end during decoding.

RangeTombstone::new has a debug_assert!(start < end) that only fires in debug builds. Corrupted on-disk data could produce invalid tombstones with start >= end in release builds, potentially causing incorrect behavior in the interval tree or filtering logic.

🛡️ Proposed validation

             tombstones.push(RangeTombstone::new(
                 UserKey::from(start_buf),
                 UserKey::from(end_buf),
                 seqno,
             ));
+            // Validate tombstone invariant
+            if let Some(rt) = tombstones.last() {
+                if rt.start >= rt.end {
+                    return Err(crate::Error::Unrecoverable);
+                }
+            }

Or validate before constructing:

+            let start_key = UserKey::from(start_buf);
+            let end_key = UserKey::from(end_buf);
+            if start_key >= end_key {
+                return Err(crate::Error::Unrecoverable);
+            }
             tombstones.push(RangeTombstone::new(
-                UserKey::from(start_buf),
-                UserKey::from(end_buf),
+                start_key,
+                end_key,
                 seqno,
             ));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/table/mod.rs` around lines 620 - 660, The decoder
(decode_range_tombstones) must validate the invariant start < end after reading
start_buf and end_buf and before calling RangeTombstone::new; if the condition
fails, return an appropriate crate::Error (e.g., a corruption/unrecoverable
error) instead of constructing the tombstone so release builds don't accept
invalid on-disk data; update the logic around
UserKey::from(start_buf)/UserKey::from(end_buf) to compare the keys and
early-return Err(...) when start >= end.

src/table/iter.rs (1)

122-122: Prefer #[expect(clippy::too_many_arguments)] over #[allow(...)].

Per coding guidelines, #[expect] should be used instead of #[allow] because it warns when the suppression becomes unnecessary (e.g., if future refactoring reduces the argument count).

♻️ Suggested fix

-    #[allow(clippy::too_many_arguments)]
+    #[expect(clippy::too_many_arguments)]

As per coding guidelines: "Prefer #[expect(lint)] over #[allow(lint)] — #[expect] warns when suppression becomes unnecessary"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/table/iter.rs` at line 122, Replace the
#[allow(clippy::too_many_arguments)] attribute on the function/impl annotated in
this diff with #[expect(clippy::too_many_arguments)] so the suppression still
applies but will warn if it becomes unnecessary; locate the exact attribute
instance (the attribute decorating the function that currently uses
#[allow(clippy::too_many_arguments)]) and change the attribute name only,
leaving the lint and surrounding code unchanged.

src/abstract_tree.rs (1)

239-245: Consider #[expect] instead of #[warn] for lint attribute.

The #[warn(clippy::type_complexity)] attribute is unusual here — it would emit warnings rather than suppress them. If the intent is to suppress the lint, use #[expect(clippy::type_complexity)] or #[allow(clippy::type_complexity)].

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/abstract_tree.rs` around lines 239 - 245, The attribute on function
flush_to_tables uses #[warn(clippy::type_complexity)] which will emit warnings
instead of suppressing them; change it to #[expect(clippy::type_complexity)] (or
#[allow(clippy::type_complexity)] if you prefer permanent suppression) placed on
the flush_to_tables function declaration so the clippy type_complexity lint is
treated as expected/allowed for the flush_to_tables / flush_to_tables_with_rt
code path.

src/range.rs (1)

101-101: Prefer #[expect(clippy::too_many_lines)] over #[allow(...)].

Per coding guidelines, use #[expect] so you get a warning when the suppression becomes unnecessary.

♻️ Suggested fix

-    #[allow(clippy::too_many_lines)]
+    #[expect(clippy::too_many_lines)]

As per coding guidelines: "Prefer #[expect(lint)] over #[allow(lint)] — #[expect] warns when suppression becomes unnecessary"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/range.rs` at line 101, Replace the attribute
#[allow(clippy::too_many_lines)] with #[expect(clippy::too_many_lines)] to
follow the coding guideline; locate the attribute (attached to the
function/module declared near the top of src/range.rs) and change the lint
suppression to an expectation so you receive a warning if the suppression
becomes unnecessary.

src/tree/mod.rs (1)

778-816: Performance concern: Linear scan of all SST tables for range tombstone suppression.

The current implementation iterates over every table in every level (lines 802-813) to check range tombstones. For point reads, this could be expensive with many tables.

Consider filtering tables by key range before checking their tombstones, similar to how get_for_key filters tables in get_internal_entry_from_tables. The PR notes this as a known limitation ("linear SST scan for point-read suppression").

💡 Potential optimization sketch

-        for table in super_version
-            .version
-            .iter_levels()
-            .flat_map(|lvl| lvl.iter())
-            .flat_map(|run| run.iter())
-        {
+        // Only check tables whose key range contains the key
+        for table in super_version
+            .version
+            .iter_levels()
+            .flat_map(|lvl| lvl.iter())
+            .filter_map(|run| run.get_for_key(key))
+        {
             for rt in table.range_tombstones() {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/tree/mod.rs` around lines 778 - 816, The
is_suppressed_by_range_tombstones function currently scans every SST table;
limit that by first filtering tables whose key range could contain the target
key (like get_for_key / get_internal_entry_from_tables does) before iterating
range_tombstones(): for each table from
super_version.version.iter_levels().flat_map(...).flat_map(...), check the
table's key range (e.g., smallest/largest key or a key_range() accessor on the
Table) and skip tables where key is outside that range, then only call
table.range_tombstones() and rt.should_suppress; keep the existing
active_memtable/sealed_memtables checks and ensure you use the same table-range
helpers used elsewhere to avoid duplicating range logic.

src/table/mod.rs (1)

648-666: Consider preserving error context for easier debugging.

The map_err(|_| crate::Error::Unrecoverable) discards the original I/O error. While Unrecoverable is semantically appropriate for corrupted data, preserving context (e.g., which field failed, byte offset) could aid debugging in production.

💡 Optional: Add context to decode errors

If a more descriptive error variant exists or is added later (e.g., Error::RangeTombstoneCorrupted { offset, reason }), consider using it here to preserve the cursor position and failure reason.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/table/mod.rs` around lines 648 - 666, The current reads
(cursor.read_u16::<LE>(), cursor.read_exact(...), cursor.read_u64::<LE>()) map
all IO errors to crate::Error::Unrecoverable, discarding the original error and
offset; update these map_err calls to capture and wrap the original error and
context (e.g., which field: "start_len"/"start_buf"/"end_len"/"end_buf"/"seqno"
and the cursor position/byte offset) into a more informative Error variant or
into Unrecoverable with a message, so failures when reading start_len,
start_buf, end_len, end_buf, and seqno preserve the underlying io::Error and an
indication of the failing field/offset for easier debugging.

src/table/util.rs (1)

31-32: ⚠️ Potential issue | 🟡 Minor

Use cfg_attr(..., expect(...)) instead of a permanent #[allow].

The feature-gated parameter count is exactly what cfg_attr(feature = "metrics", expect(...)) is for. Leaving #[allow] here suppresses the lint in the one build where it should still fire.

Suggested patch

-// NOTE: #[allow] not #[expect] because arg count changes with cfg(feature = "metrics")
-#[allow(clippy::too_many_arguments)]
+#[cfg_attr(
+    feature = "metrics",
+    expect(
+        clippy::too_many_arguments,
+        reason = "metrics adds the extra parameter; without that feature this stays at the lint threshold"
+    )
+)]
 pub fn load_block(

As per coding guidelines, "Prefer #[expect(lint)] over #[allow(lint)] — #[expect] warns when suppression becomes unnecessary."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/table/util.rs` around lines 31 - 32, Replace the permanent attribute
#[allow(clippy::too_many_arguments)] with a conditional expect so the lint still
fires when the metrics feature is disabled; specifically change the attribute to
#[cfg_attr(feature = "metrics", expect(clippy::too_many_arguments))] (or the
inverse depending on feature semantics) at the same item (the attribute applied
in src/table/util.rs) so the suppression only applies under the cfg where extra
arguments exist and the lint will warn in builds where it should.

src/range.rs (2)

302-307: Strengthen the fast path to skip wrapping when no tombstone is visible.

A non-empty all_range_tombstones can still have zero entries visible at seqno, so this currently wraps with a no-op filter in those cases.

⚡ Proposed patch

-            // Fast path: skip filter wrapping when no range tombstones exist
-            if all_range_tombstones.is_empty() {
+            // Fast path: skip wrapping when no tombstone is visible for this read seqno
+            if !all_range_tombstones.iter().any(|rt| rt.visible_at(seqno)) {
                 Box::new(iter)
             } else {
                 Box::new(RangeTombstoneFilter::new(iter, all_range_tombstones, seqno))
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/range.rs` around lines 302 - 307, The fast-path should skip wrapping not
only when all_range_tombstones.is_empty() but also when none of the tombstones
are visible at the given seqno; update the conditional around Box::new(...) to
inspect all_range_tombstones for any entry visible at seqno (e.g. using the
visibility predicate/method used by RangeTombstoneFilter or by
filtering/map-checking the collection) and only construct
RangeTombstoneFilter::new(iter, all_range_tombstones, seqno) if at least one
tombstone is visible; otherwise return Box::new(iter) unchanged.

---

176-196: Remove redundant tombstone sorting in this path.

rts.sort() here is duplicate work: RangeTombstoneFilter::new already sorts the forward list (and builds a reverse-sorted copy). This adds avoidable O(n log n) cost per range open.

♻️ Proposed patch

             let all_range_tombstones = if rt_count > 0 {
                 let mut rts: Vec<RangeTombstone> = Vec::with_capacity(rt_count);

                 rts.extend(lock.version.active_memtable.range_tombstones_sorted());
                 for mt in lock.version.sealed_memtables.iter() {
                     rts.extend(mt.range_tombstones_sorted());
                 }
                 if let Some((mt, _)) = &lock.ephemeral {
                     rts.extend(mt.range_tombstones_sorted());
                 }
                 for table in lock
                     .version
                     .version
                     .iter_levels()
                     .flat_map(|lvl| lvl.iter())
                     .flat_map(|run| run.iter())
                 {
                     rts.extend(table.range_tombstones().iter().cloned());
                 }
-                rts.sort();
                 rts
             } else {
                 Vec::new()
             };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/range.rs` around lines 176 - 196, The vector `rts` constructed for
`all_range_tombstones` is being sorted with `rts.sort()` redundantly;
`RangeTombstoneFilter::new` already sorts the forward list and produces the
reverse-sorted copy, so remove the `rts.sort()` call in the block that builds
`rts` (the code that extends from
`lock.version.active_memtable.range_tombstones_sorted()`, sealed memtables,
`lock.ephemeral`, and version runs) and leave the unsorted `rts` to be passed
into `RangeTombstoneFilter::new`; this avoids the extra O(n log n) work while
preserving correctness for `RangeTombstone` handling.

.github/copilot-instructions.md (1)

65-74: ⚠️ Potential issue | 🟠 Major

Narrow this rule to PR-visible call sites and preserve unconditional Tier 1/2 reporting.

Line 69 (“check every call site”) conflicts with Line 9 diff-only review scope, and Line 73’s “not a bug” wording is too absolute. Please scope this to call sites visible in the PR diff and explicitly keep Tier 1/Tier 2 findings reportable even when a local protocol exists.

Suggested wording tweak

- **Before flagging any potential issue, trace the FULL call chain:**
+ **Before flagging any potential issue, trace the call chain visible in the PR diff:**
...
- **Only flag an issue if it survives all three checks.** A finding that looks like a bug in one function but is handled by its caller is not a bug — it is a protocol between the two functions.
+ **Only suppress Tier 3/Tier 4 findings if they fail these checks.** If the pattern still indicates Tier 1 (correctness) or Tier 2 (safety/crash-recovery) risk, flag it even when caller/callee protocols are documented.

Based on learnings: “Review ONLY code within the PR's diff” and “Tier 1/Tier 2 issues must be flagged regardless of documentation.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/copilot-instructions.md around lines 65 - 74, Update the "Design
Decision Analysis (CRITICAL)" guidance to limit the "check every call site"
requirement to only call sites that are visible within the PR diff and to
explicitly preserve unconditional reporting for Tier 1 and Tier 2 findings; edit
the sentences that mention "check every call site" and "Only flag an issue if it
survives all three checks" so they read that reviewers should trace the full
call chain only among PR-visible call sites and that documented local protocols
do not suppress Tier 1/Tier 2 reports.

src/range_tombstone_filter.rs (1)

151-261: Rename the new unit tests to the repo’s standard pattern.

Several of these names (no_tombstones, basic_suppression, half_open_end_exclusive, reverse_half_open, …) are outside the repository’s what_condition_expected convention, which makes the new test surface less consistent than the rest of the crate.

As per coding guidelines, "Use test naming convention: fn <what>_<condition>_<expected>()."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/range_tombstone_filter.rs` around lines 151 - 261, Rename the tests to
follow the <what>_<condition>_<expected> pattern: no_tombstones ->
items_no_tombstones_return_all, basic_suppression ->
items_with_range_tombstone_suppress_intermediate_keys,
tombstone_does_not_suppress_newer_kv ->
tombstone_older_than_kv_does_not_suppress, half_open_end_exclusive ->
range_end_exclusive_suppresses_middle_keys, multiple_overlapping_tombstones ->
overlapping_tombstones_keep_newer_kv, tombstone_not_visible_at_read_seqno ->
tombstone_newer_than_read_seqno_not_visible, reverse_basic_suppression ->
rev_items_with_range_tombstone_suppress_middle_keys, reverse_half_open ->
rev_range_end_exclusive_suppresses_between; update the fn identifiers
accordingly (e.g., fn items_no_tombstones_return_all()) and keep the test bodies
unchanged so references to RangeTombstoneFilter, rt, kv, SeqNo::MAX remain
valid.

.github/copilot-instructions.md (1)

73-73: Clarify suppression wording to avoid reviewer misinterpretation

The sentence can be read both ways. Consider rewording to explicitly say Tier 3/4 findings may be suppressed only when they do not survive the 3 checks.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/copilot-instructions.md at line 73, The sentence "Only suppress Tier
3/Tier 4 findings if they fail these checks." is ambiguous; update the wording
in .github/copilot-instructions.md to explicitly state that Tier 3/Tier 4
findings may be suppressed only when they do not survive the three checks (e.g.,
"Only suppress Tier 3/Tier 4 findings if they do not survive the three checks
below: ..."), making clear that any finding that still indicates Tier 1 or Tier
2 must be flagged regardless of suppression rationale.